Skip to content

apps/testing:move arch_libc, fmemopen, scanftest folders to the new libc folder#2964

Merged
xiaoxiang781216 merged 1 commit into
apache:masterfrom
txy-21:merge-libc-folder
Jan 22, 2025
Merged

apps/testing:move arch_libc, fmemopen, scanftest folders to the new libc folder#2964
xiaoxiang781216 merged 1 commit into
apache:masterfrom
txy-21:merge-libc-folder

Conversation

@txy-21

@txy-21 txy-21 commented Jan 22, 2025

Copy link
Copy Markdown
Contributor

Summary

This is one step that merge test case as discussed in #2931.
Create libc folder and move arch_libc, fmemopen, scanftest folders to the new libc folder.

Impact

Only affect apps/testing folder

New folder :
libc

Move the following folder:
arch_libc, fmemopen, scanftest

Testing

CI test

…ib folder

Signed-off-by: tengshuangshuang <tengshuangshuang@xiaomi.com>
@nuttxpr

nuttxpr commented Jan 22, 2025

Copy link
Copy Markdown

[Experimental Bot, please feedback here]

Yes, this PR appears to meet the basic NuttX requirements, though it could be more thorough. Here's a breakdown:

Strengths:

  • Clear Summary: The summary explains the "why" (part of a larger effort), "what" (moving test folders), and "how" (creating a new libc folder). The link to the related PR is excellent.
  • Impact Description: Clearly states the limited impact to the apps/testing folder. Lists the new and moved folders.
  • Testing Mention: Mentions CI testing.

Weaknesses:

  • Missing Detail in Impact: While it mentions affected folders, it doesn't discuss why this change is being made. What benefit does restructuring bring? This helps reviewers understand the motivation. Also, explicitly stating "NO" for the other impact categories would be clearer.
  • Insufficient Testing Detail: Simply saying "CI test" isn't enough. Which CI targets? Were any manual tests performed? Providing snippets of successful test output (even if just "pass") would strengthen the testing section significantly. The template requests "before" and "after" logs, which are missing. While not strictly required for pure restructuring, some evidence of functionality not being broken is good practice.
  • Missing Build Host Information: The template asks for build host details (OS, compiler, etc.). This is missing.

Recommendation:

While the PR seems to meet the letter of the requirements, adding the missing details, especially regarding testing and build environment, would significantly improve its quality and make review easier. Even a simple "Built and tested on Linux with GCC 12.x, CI passed on sim:nsh" would be a substantial improvement. Explaining the rationale behind the restructuring would also be beneficial.

@cederom cederom left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @txy-21 :-)

@xiaoxiang781216 xiaoxiang781216 merged commit 57c8a62 into apache:master Jan 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants